Unify workspace tool surface tabs#38
Conversation
Add terminal and browser new-tab controls to dialog/window tool workbenches. Register window-host tool instances for cwd sync, keep git diff as a replaceable dialog document, and replay terminal scrollback without echoing xterm responses back into the PTY.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive workspace tools system enabling docked and windowed tool surfaces (terminal, browser, file preview, git diffs). It refactors terminal IPC to support multi-subscriber sessions with scrollback buffering, adds workspace browser view management via Electron WebContentsView, and integrates a unified tool UI into the workspace sidebar and window hosts. ChangesTerminal Session Refactoring
Workspace Browser View System
Workspace Tools Surface and Management System
Infrastructure and Global Wiring
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf74c0b009
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
desktop/src/main/features/workspace-browser/ipc.ts (1)
264-275: HandleloadURL()rejections for diagnostics/recovery.
webContents.loadURL()rejects on navigation failures, but Electron already attaches a noop rejection handler, so this won’t typically create a main-process unhandled-rejection crash. Still, adding a.catch()is a low-effort way to ensure failures are observed/logged and to support any recovery behavior beyond thedid-fail-loadstate updates.🛡️ Proposed fix
- void instance.view.webContents.loadURL(nextUrl); + void instance.view.webContents.loadURL(nextUrl).catch(() => { + // `did-fail-load` updates state; avoid an unhandled rejection in main. + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/features/workspace-browser/ipc.ts` around lines 264 - 275, The loadWorkspaceBrowserUrl function calls instance.view.webContents.loadURL(...) without handling promise rejections; update both places where loadURL is called in loadWorkspaceBrowserUrl to attach a .catch() that logs the error (e.g., console.error or the app logger) and optionally triggers any recovery/diagnostic behavior (such as emitting an IPC/event or calling instance/WorkspaceBrowserInstance recovery methods) so navigation failures are observed and can be acted on.desktop/src/main/features/workspace-tools/service.ts (1)
115-115: ⚡ Quick winThe fallback to
treePathInputappears unreachable.
resolveWorkspaceTreePaththrows ifabsolutePathwould be outside the workspace root. SinceabsolutePathToTreePathonly returnsnullwhen the relative path escapes the workspace, this fallback on line 115 should never execute.♻️ Proposed simplification
- const treePath = absolutePathToTreePath(root, absolutePath) ?? treePathInput; + const treePath = absolutePathToTreePath(root, absolutePath)!;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/main/features/workspace-tools/service.ts` at line 115, The fallback to treePathInput when computing treePath is unreachable because absolutePathToTreePath only returns null for paths that escape the workspace and resolveWorkspaceTreePath already throws for those cases; remove the unreachable "?? treePathInput" fallback and assign treePath directly from absolutePathToTreePath(root, absolutePath), or alternatively handle the out-of-workspace case explicitly where resolveWorkspaceTreePath is called (adjust the call sites of absolutePathToTreePath and resolveWorkspaceTreePath rather than keeping the redundant fallback).desktop/src/renderer/app/router.tsx (1)
22-25: 💤 Low valueConsider inlining the trivial wrapper component.
The
WorkspaceToolWindowRoutePagewrapper (lines 43-45) adds no behavior and could be replaced by directly rendering<WorkspaceToolWindowPage />inline, consistent with other simple routes in this file (e.g., line 16-18).♻️ Proposed simplification
- <Route - component={WorkspaceToolWindowRoutePage} - path="/workspace-tools" - /> + <Route path="/workspace-tools"> + <WorkspaceToolWindowPage /> + </Route>Remove the wrapper function:
-function WorkspaceToolWindowRoutePage() { - return <WorkspaceToolWindowPage />; -} -Also applies to: 43-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@desktop/src/renderer/app/router.tsx` around lines 22 - 25, Remove the trivial wrapper WorkspaceToolWindowRoutePage and inline WorkspaceToolWindowPage in the Route so the route matches the simple-route pattern used elsewhere: replace the Route using component={WorkspaceToolWindowRoutePage} with the same inline form used in other routes (e.g., component={WorkspaceToolWindowPage} or element={<WorkspaceToolWindowPage />} to match the file's routing API) and delete the now-unused WorkspaceToolWindowRoutePage function/definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@desktop/src/main/features/workspace-browser/ipc.ts`:
- Around line 264-275: Reject any non-http(s) URL (except "about:blank") in
loadWorkspaceBrowserUrl by validating nextUrl before calling
instance.view.webContents.loadURL; allow nextUrl === "about:blank" or use the
existing isHttpUrl helper (or new URL parsing) to permit only http/https
schemes, and if validation fails do not call loadURL (set instance.url to
"about:blank" or leave unchanged and return early, optionally emit/log a
warning) so renderer-driven IPC cannot cause non-http(s) navigations.
In `@desktop/src/renderer/app/workspace/workspace-tool-host.tsx`:
- Around line 349-381: The closeDynamicTab callback currently voids the Promise
from window.workspaceBrowser.destroy, losing errors; update the call in
closeDynamicTab so destruction errors are handled and logged (e.g., replace the
void call with window.workspaceBrowser.destroy({ browserViewId:
tab.browserViewId }).catch(err => console.error('Failed to destroy browser
view', { browserViewId: tab.browserViewId, tabId: tab.id, err }))); keep the
rest of closeDynamicTab logic the same so UI state updates proceed even if
destruction fails.
- Around line 989-1071: The getState, navigate, goBack, goForward and reload
calls currently swallow errors in their .catch(() => {}) blocks; update each
catch to log the error (including context) instead of ignoring it — for example
replace the empty catches on window.workspaceBrowser.getState, .navigate,
.goBack, .goForward and .reload with processLogger.error or console.error calls
that include the operation name, tab.browserViewId (or tab.id) and the caught
error; keep behavior otherwise (still call
handleStateChange/setBrowserState/updateBrowserTab as before).
In `@desktop/src/renderer/app/workspace/workspace-tool-store.ts`:
- Around line 89-96: The catch block on the promise from
window.desktopWindow.getWorkspaceToolSurfaceState() is swallowing errors; update
the .catch to log the error before marking hydration complete so failures are
visible. Specifically, in the promise chain that calls
getWorkspaceToolSurfaceState() and then syncWorkspaceToolState(state), change
the .catch handler to accept an error parameter and log it (e.g., console.error
or the app logger) and then call useWorkspaceToolStore.setState({ hydrated: true
}) so visibility is preserved while keeping behavior the same.
In `@desktop/src/shared/workspace-browser.ts`:
- Around line 76-77: The exposed IPC method return types for destroy and detach
are too broad (Promise<unknown>); update the WorkspaceBrowser interface
signatures for destroy and detach to return Promise<{ ok: true }> instead so
they match the actual responses sent on WORKSPACE_BROWSER_DESTROY_CHANNEL and
WORKSPACE_BROWSER_DETACH_CHANNEL; modify the function/type declarations named
destroy and detach in workspace-browser.ts to use the narrowed return type.
---
Nitpick comments:
In `@desktop/src/main/features/workspace-browser/ipc.ts`:
- Around line 264-275: The loadWorkspaceBrowserUrl function calls
instance.view.webContents.loadURL(...) without handling promise rejections;
update both places where loadURL is called in loadWorkspaceBrowserUrl to attach
a .catch() that logs the error (e.g., console.error or the app logger) and
optionally triggers any recovery/diagnostic behavior (such as emitting an
IPC/event or calling instance/WorkspaceBrowserInstance recovery methods) so
navigation failures are observed and can be acted on.
In `@desktop/src/main/features/workspace-tools/service.ts`:
- Line 115: The fallback to treePathInput when computing treePath is unreachable
because absolutePathToTreePath only returns null for paths that escape the
workspace and resolveWorkspaceTreePath already throws for those cases; remove
the unreachable "?? treePathInput" fallback and assign treePath directly from
absolutePathToTreePath(root, absolutePath), or alternatively handle the
out-of-workspace case explicitly where resolveWorkspaceTreePath is called
(adjust the call sites of absolutePathToTreePath and resolveWorkspaceTreePath
rather than keeping the redundant fallback).
In `@desktop/src/renderer/app/router.tsx`:
- Around line 22-25: Remove the trivial wrapper WorkspaceToolWindowRoutePage and
inline WorkspaceToolWindowPage in the Route so the route matches the
simple-route pattern used elsewhere: replace the Route using
component={WorkspaceToolWindowRoutePage} with the same inline form used in other
routes (e.g., component={WorkspaceToolWindowPage} or
element={<WorkspaceToolWindowPage />} to match the file's routing API) and
delete the now-unused WorkspaceToolWindowRoutePage function/definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20b36e52-3d28-4166-b673-57c315baa5bf
📒 Files selected for processing (27)
desktop/src/main/features/terminal/ipc.tsdesktop/src/main/features/workspace-browser/ipc.tsdesktop/src/main/features/workspace-tools/ipc.tsdesktop/src/main/features/workspace-tools/service.tsdesktop/src/main/ipc/register.tsdesktop/src/main/windows/workspace-tool-window.tsdesktop/src/preload/bridges/desktop-window.tsdesktop/src/preload/bridges/terminal.tsdesktop/src/preload/bridges/workspace-browser.tsdesktop/src/preload/index.tsdesktop/src/renderer/app/router.tsxdesktop/src/renderer/app/workspace/workspace-browser-view.tsxdesktop/src/renderer/app/workspace/workspace-page.tsxdesktop/src/renderer/app/workspace/workspace-right-sidebar.tsxdesktop/src/renderer/app/workspace/workspace-terminal-view.tsxdesktop/src/renderer/app/workspace/workspace-tool-host.tsxdesktop/src/renderer/app/workspace/workspace-tool-store.tsdesktop/src/renderer/app/workspace/workspace-ui-store.tsdesktop/src/renderer/env.d.tsdesktop/src/renderer/platform/api-client.tsdesktop/src/renderer/platform/query-keys.tsdesktop/src/shared/desktop-window.tsdesktop/src/shared/terminal.tsdesktop/src/shared/workspace-browser.tsdesktop/src/shared/workspace-tool-instances.tsdesktop/src/shared/workspace-tool-surface.tsdesktop/src/shared/workspace-tools.ts
💤 Files with no reviewable changes (1)
- desktop/src/renderer/app/workspace/workspace-ui-store.ts
There was a problem hiding this comment.
💡 Codex Review
This is separate from preview reads: the new untracked-diff path follows untracked symlinks before generating the Git panel patch. If a workspace contains an untracked symlink like secret -> /etc/passwd, fs.stat() follows it and this readFile() includes the target contents in the displayed diff, even though Git would treat the symlink itself as a small file containing the link target. Use lstat/reject symlinks or realpath containment before reading untracked files.
When the selected project is a subdirectory of a larger Git repo, this diff is run from gitRoot without a pathspec, while the status list above is explicitly filtered back to root. In that context the Git tools tab will show staged/unstaged changes from sibling directories outside the selected project, so users can review unrelated changes under the wrong workspace. Pass the root-relative path (after --) to both git diff calls.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| webContents.setWindowOpenHandler(({ url }) => { | ||
| if (isHttpUrl(url)) { | ||
| loadWorkspaceBrowserUrl(instance, url); | ||
| } else if (isAllowedExternalUrl(url)) { | ||
| void shell.openExternal(url); | ||
| } | ||
| return { action: "deny" }; |
There was a problem hiding this comment.
Block non-HTTP main-frame navigations
This only handles window.open, so a loaded page can still drive the current workspace browser via a top-level redirect or location.href to a non-HTTP URL and bypass the workspaceBrowserLoadUrl() check used for user-entered URLs. In that scenario the WebContentsView attempts the navigation and later records the resulting URL through did-navigate, so add a will-navigate/redirect guard that enforces the same http(s)/about:blank policy for main-frame navigations.
Useful? React with 👍 / 👎.
Summary
Verification
Summary by CodeRabbit